Skip to content

Conversation

@reedlepee123
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these comments are necessary since we (the reviewers) can see exactly what you changed from the diff (this view).

Can you remove them please?

@huonw
Copy link
Contributor

huonw commented Oct 20, 2013

Thank you for this, it looks pretty good!

I'd like the removal of the comments like // all already priv, since this information is already accessible from the git history.

Also, this needs to be rebased on top of the master branch of the mozilla/rust repo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that these are designed for use outside this module (but I'm not sure).

If they are, then the should still be public.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I just checked in servo, and it does use the fields directly, so these need to be public. (git checkout src/libextra/url.rs should work.)

@reedlepee123
Copy link
Contributor Author

I removed unnecessary comments and white spaces and changed some fields to public as suggested in above comments .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space.

@huonw
Copy link
Contributor

huonw commented Oct 20, 2013

It's almost ready to go! :) Although it still needs to be rebased.

(cc @catamorphism.)

@bstrie
Copy link
Contributor

bstrie commented Oct 20, 2013

@reedlepee123 You'll need to rebase and fix the merge conflicts before we can approve this. Find either myself or tjc on IRC if you would like some instruction on how to do this.

@bstrie
Copy link
Contributor

bstrie commented Oct 22, 2013

This closes #4386.

@bors bors closed this Oct 22, 2013
@bors bors merged commit 7e6f5bb into rust-lang:master Oct 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants